Skip to content
This repository has been archived by the owner on Oct 7, 2021. It is now read-only.

Use matched count to determine server_is_available #185

Merged
merged 2 commits into from
Aug 24, 2017

Conversation

b3em
Copy link
Collaborator

@b3em b3em commented Aug 23, 2017

  • change the signature of internal server_is_available, removed the count_xxx function pointer
  • the server_is_available function now use publication_matched and subscription_matched functions
  • DCPSPublication and DCPSSubscription listeners add partition name for discovered topics

- change the signature of internal server_is_available, removed the count_xxx function pointer
- the server_is_available function now use publication_matched and subscription_matched functions
- DCPSPublication and DCPSSubscription listeners add partition name for discovered topics
@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Aug 23, 2017
@dirk-thomas dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 23, 2017
@dirk-thomas
Copy link
Member

LGTM. Do you want me to trigger a CI build for this? Otherwise please feel free to merge this.

@b3em
Copy link
Collaborator Author

b3em commented Aug 23, 2017

Yes please I like a CI build, tested locally and build fine just like to make sure I don't push anything broken into master

@dirk-thomas
Copy link
Member

Build Status

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I wasn't aware of get_publication_matched_status(), I thought we would need to have a listener and keep track of it ourselves. We should look at using this function in our other rmw implementations.

@b3em
Copy link
Collaborator Author

b3em commented Aug 23, 2017

I agree, the current listener implementation adds the topic_names (incl partition) to a list and the count functions return the count of publishers/subscribers based on the topic_name match with the list. This check is incomplete, QoS also needs to be compatible in order to count matches.
Following functions are used to get the count:

rmw_ret_t rmw_count_subscribers(const rmw_node_t * node, const char * topic_name, size_t * count);
rmw_ret_t rmw_count_publishers(const rmw_node_t * node,  const char * topic_name, size_t * count);

I suggest adding new functions (and maybe even remove the previous count functions)

rmw_ret_t rmw_subscriber_count(const rmw_subscriber_t *, size_t *);
rmw_ret_t rmw_publisher_count(const rmw_publisher_t *, size_t *);

with these function you could then get the actual number of matching publishers/subscribers. Extra information available in the get_publication_matched_status() could also be added.

@dirk-thomas
Copy link
Member

It would indeed be good to add this to the rmw API. I created a ticket based on your earlier feedback: ros2/rmw#120.

I am not sure if you would like to contribute this addition to the rmw interface as well as expose these functions afterwards in rcl*?

@b3em b3em merged commit 13d1214 into master Aug 24, 2017
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Aug 24, 2017
@b3em
Copy link
Collaborator Author

b3em commented Aug 24, 2017

I can, but first need to make sure that the beta3-release fixes for opensplice are ready.

@b3em
Copy link
Collaborator Author

b3em commented Aug 24, 2017

Think I should have rebased instead

@b3em b3em deleted the update_server_is_available branch August 24, 2017 07:29
@dirk-thomas
Copy link
Member

I can, but first need to make sure that the beta3-release fixes for opensplice are ready.

Absolutely. Before you look at the rmw ticket please comment there so other are aware of it to avoid duplicate efforts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants